Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

RP1 Audio Out #6668

Open
wants to merge 4 commits into
base: rpi-6.12.y
Choose a base branch
from
Open

RP1 Audio Out #6668

wants to merge 4 commits into from

Conversation

njhollinghurst
Copy link
Contributor

@njhollinghurst njhollinghurst commented Feb 13, 2025 •
edited
Loading

Candidate version of an RP1 Audio Out driver

48kHz stereo only. GPIO 12/13 only.

Now rewritten as an ASOC DAI driver; a generic sound card and "dummy" codec must also be enabled via OF.

@njhollinghurst njhollinghurst force-pushed the audio_out branch 3 times, most recently from aa9d461 to aec0c90 Compare February 13, 2025 16:07
@njhollinghurst
Copy link
Contributor Author

njhollinghurst commented Feb 17, 2025 •
edited
Loading

@pelwell could you cast an eye over the first two commits please (since renamed for rebase), to check they don't break I2S or anything else? Thanks.

@pelwell
Copy link
Contributor

pelwell commented Feb 17, 2025

Do you need a card to test with?

@njhollinghurst
Copy link
Contributor Author

njhollinghurst commented Feb 17, 2025 •
edited
Loading

Successfully tested JustBoom DAC Zero and IQaudIO Pi-Codec+ hats at various sample rates.

I could even load and use the rp1aout driver at the same time, but (predictably) I2S stomped on rp1aout's clock when using the (crystal-less) JustBoom hat; they did seem able to coexist with the IQaudIO.

Getting 48000Hz and 44100Hz-based rates at the same time is impractical with a single audio PLL. Running two outputs at (48000*32, 48000*3200) ought to be achievable, but the RP1 clk driver doesn't currently oblige. It's not a vital use case.

P33M
P33M reviewed Feb 18, 2025
View reviewed changes
sound/soc/raspberrypi/rp1_aout.c
cmd == SNDRV_PCM_TRIGGER_SUSPEND ||
cmd == SNDRV_PCM_TRIGGER_PAUSE_PUSH) {
/* Push a zero sample (assuming DMA has stopped already?) */
aout_reg_wr(aout, AUDIO_OUT_SAMPLE_FIFO, 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Underflow should be handled gracefully by the hardware (the last sample plays forever) - ASoC decouples DMA ops from driver visibility so you may introduce a glitch here if DMA is still running. I don't know if it synchronously drains the DMA buffers or flushes them.

Copy link
Contributor Author

@njhollinghurst njhollinghurst Feb 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In tests it does seem to use the given value quite reliably -- I'll remove the question mark

It wasn't clear to me whether a step to zero would be more or less objectionable than a nonstandard DC value (perhaps followed by a step later). Maybe 'stop' and 'pause' cases should be treated differently?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After a bit more debug: I haven't actually observed a PAUSE_PUSH or SUSPEND -- when I Ctrl-Z aplay, it does a STOP. I think I will keep the zero as it seems to work, and may be better than sticking at an arbitrary value.

@njhollinghurst njhollinghurst force-pushed the audio_out branch 2 times, most recently from b81599d to 0ab48b8 Compare February 18, 2025 18:47
@P33M
Copy link
Contributor

P33M commented Feb 19, 2025

Rebooting with the carrier active doesn't result in a clean shutdown - it goes thump. snd_soc_dai_ops needs a shutdown callback...

@njhollinghurst
Copy link
Contributor Author

njhollinghurst commented Feb 19, 2025 •
edited
Loading

Rebooting with the carrier active doesn't result in a clean shutdown - it goes thump. snd_soc_dai_ops needs a shutdown callback...

I think platform shutdown is the one I want: DAI shutdown happens after every stream, and we decided we wanted to leave the "bias" on in those cases. But I am currently battling a suspected race condition here...

@njhollinghurst
Copy link
Contributor Author

Created a tag https://github.com/njhollinghurst/linux/tree/audio_out_wip with current history.
Merciless squashing will now ensue.

@njhollinghurst
arm64: dts: Add the Audio Out block to rp1.dtsi
1aee1eb 
Change the standard rate of PLL_AUDIO_SEC from 192MHz to
153.6MHz to suit audio out.

Declare audio out hardware and give it a named pin control.

Signed-off-by: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com>
@njhollinghurst
clk: rp1: Allow audio out to use PLL_AUDIO_SEC; workaround rounding e…
0015b33 
…rror

Connect PLL_AUDIO_SEC to CLK_AUDIO_OUT, which had been commented out
to avoid interference with I2S: we expect them never to be enabled
at the same time. Work around a rounding error that occurs when the
desired rate is exactly the max but not exactly achievable by the PLL.

Signed-off-by: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com>
@njhollinghurst njhollinghurst force-pushed the audio_out branch 3 times, most recently from 37d7eca to d16651b Compare February 19, 2025 20:12
@njhollinghurst njhollinghurst changed the title Audio out (WIP) RP1 Audio Out Feb 19, 2025
@njhollinghurst njhollinghurst marked this pull request as ready for review February 19, 2025 20:17
@njhollinghurst njhollinghurst marked this pull request as draft February 20, 2025 11:01
@njhollinghurst
Copy link
Contributor Author

njhollinghurst commented Feb 20, 2025 •
edited
Loading

Sigh... Something seems to go wrong on about 1 boot in 20. I get this error:

pi@raspberrypi:~ $ aplay -v arpanet_20201206.raw -D hw:CARD=RP1AudioOut -f dat
aplay: main:831: audio open error: Invalid argument

The DAI driver sees successful probe, startup and shutdown calls only. In the bad case there is never a call to HW params.

@pelwell
Copy link
Contributor

pelwell commented Feb 20, 2025

For extra credit, create an audremap-pi5 entry in the map that only says bcm2712, so that attempts to load it on other platforms will be blocked.

@njhollinghurst
sound: soc: raspberrypi: RP1 Audio Out driver as an ASOC DAI
167d51d 
Only 48000Hz stereo 16-bit output is currently supported.

It requires some additional OF plumbing to connect it to a
"dummy" codec and generic sound card.

Signed-off-by: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com>
@njhollinghurst
dts: overlays: Enable RP1 Audio Out using audremap-pi5-overlay
2437b72 
Since RP1 Audio Out can only work on GPIOs 12, 13 which would
previously have needed dtoverlay=audremap, overload it both to
enable and pin-map the block (do not enable for other pinouts).

At the same time, generate a default "codec" and "sound card".

Signed-off-by: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com>
@njhollinghurst
Copy link
Contributor Author

Sorry for the churn -- I think it's all good now.

  • audremap-pi5 is marked as 2712 only
  • References to runtime pm removed (as I didn't intend to implement them here/now)
  • Taking more care about spiv async register synchronization (at startup and when polling the mute FSM)
  • eschewing SET/CLR aliases for superstitious reasons (and because we hardly used and don't need them)
  • Reordered PCM and ASOC component registration, to fix a probe-time race condition.
  • After moving them around to try to influence probe order, the generic audio nodes are back at top level again.

@njhollinghurst njhollinghurst marked this pull request as ready for review February 21, 2025 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@P33M P33M P33M left review comments

@pelwell pelwell Awaiting requested review from pelwell

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@njhollinghurst @pelwell @P33M